i think it's better to move this test to HTTPClientInternalTests, this test class is intended to test public API methods

8775fc1

not sure if the preferred casing is setUrl or setURL i can go either way, or just url() can work for me too

I think setURL is the preferred casing.

Does this need to be public? I understand that redirect() needs to use it but that doesn't mean it needs to be public, does it?

I think it should be public to make it easier for users to mutate a Request they may be holding.

OK yes I guess it makes sense given a lot of the rest of the things on Request are mutable. We should have doc comments on public var url/scheme/host saying that if you want to change the property you should use setURL().

if i understand you correctly, the difference in opinion boils down to the last statement:

Sending a signal that perhaps mutating the field would lead to some logical error. .... In this case, neither is true...

the current code has some fragile logic tied to url, scheme and host and mutating the url does cause logical issues which is what this PR as well as #56 try to address. the fact we are talking about this so much should be a fairly strong signal that there is an issue here. my original suggestion was to circumvent the current logical issue and potential future ones by not allowing mutation of the url until we have a good reason to expose it

we don't gain anything by making it hard to do this, and we do push costs onto users by doing it

as far as i can judge there is not a good use case to justify exposing this to users beyond redirect which we already handle and discussed above, so imo there is no pragmatic cost here to push onto the users

this is why i still think its a better trade-off to delay exposing a mutable URL until there s a good reason to do so, hope this makes more sense now. as mentioned above i am willing to move forward with the current state of the PR for the sake of making progress

my original suggestion was to circumvent the current logical issue and potential future ones by not allowing mutation of the url until we have a good reason to expose it

And my counter argument is that there is no such thing as "not allowing mutation of the URL": the example code above does so. The question is only whether you can spell it "I'd like to change the URL", or whether you have to spell it "I'd like to create a new Request whose bits all happen to be the same as the one I already have".

I think it's really important to stress that making the url property let literally does not prevent the users from doing anything that this patch allows them to do. If you can construct a single example of what the current revision of the patch allows that cannot be achieved while the property is let, please let me know, because my understanding of Swift is that there is absolutely nothing.

@Lukasa of course! the point is not that user can't create a new request with a different url. the point is that the internal logical state of the request object is easier to manage by making url immutable, since changing the url triggers logic & validation. and since there is not a clear, pragmatic use case to make it mutable, we can get away by making it immutable and save ourselves maintenance headache, at least until a good reason shows up. hope this clarifies where i am coming from

worth mentioning again i am okay moving forward with the mutable URL option too, for the sake of making progress. so i suggest at this point we ask @artemredkin @weissi @ianpartridge and @tanner0101 to express their opinions too and just move with whatever the majority feels is the right thing, as both options are technically viable and this is more opinions

I think this just represents a fundamental disagreement here, because I don’t think there is any more difficulty in validating the URL via a setter function versus via the constructor. Regardless, though, I think we’re arguing about technicalities: anyone can achieve anything regardless of what construction is used here, so we’ve fundamentally just exerted far more energy here than we need to.

Given that it seems that I’m the only one arguing for a mutator function I think it’s easier for me to simply withdraw the objection than to poll for consensus. Let’s just move on.

I think setURL is the preferred casing.

Do we need to recalculate these? Unless we have a strong performance argument for doing so, it seems like it'd be better for scheme and host to be computed properties.

@Lukasa two reasons:

=> if we use computed properties they will need to force unwrap since host and scheme are optional on URL. this means we rely on the fact we validated they are not empty in the url setter, which we do now. so could work. having said that, in my experience this would be a fragile design as someone in the future may decide to change the validation and forget there are computed properties that make this assumption leading to runtime errors. the current implementation makes this very explicit and hard to get wrong in the future

=> we are lowercasing the scheme which is used for business logic and validation downstream code. using the url's schema will not include this manipulation and can cause business logic issues. we could instead create a new url object with the lowercased schema but the current implementation seems less intrusive and expensive

I think the first reason is more compelling than the second. Maintaining the case of the URL scheme is not worthwhile, and so creating and storing the new URL object with the lowercased URL scheme seems reasonable enough to me.

The bigger concern for me is your worry about the business logic changing and leading to runtime errors. It seems to me that the correct defense against that risk is a unit test that validates that it is impossible to create a request with a missing host or scheme. No matter what the implementation of the host/scheme getter, as long as that test continues to pass then by definition they cannot fail to obtain a value.

The argument about fragile design can be made in the opposite direction, incidentally. If a user changes the validation logic they may forget to update scheme or host, which will then lead to separate business logic issues. My general assumption is that it is better to have a single source of truth than to split it.

That said, if you feel strongly I'm ok to let this slide. In that case, the only note I have is that self._schema is incorrectly named, and it should be self._scheme.

i could go either way on this one, no strong opinion. @artemredkin @ianpartridge @tanner0101 ?

Tbh I don't think it's worth spending the cycles discussing it. Let's leave it as is, fixing the typo of schema to scheme.

Does this need to be public? I understand that redirect() needs to use it but that doesn't mean it needs to be public, does it?

Shouldn't this be _scheme not _schema?

Johannes suggest to drop version from request completely (see discussion in #56)

even better! done in 73c400f

does method have to be let? It's not part of the url change verification, so we can keep it as var (maybe move closer to all other vars in the file?

I think having something like:

var request = try HTTPClient.Request(url: redirectURL, method: method, headers: headers, body: body)
...
if convertToGet {
    request.method = .GET
    request.body = nil
    request.headers.remove(name: "Content-Length")
    request.headers.remove(name: "Content-Type")
}

if !originalURL.hasTheSameOrigin(as: redirectURL) {
    request.headers.remove(name: "Origin")
    request.headers.remove(name: "Cookie")
    request.headers.remove(name: "Authorization")
    request.headers.remove(name: "Proxy-Authorization")
}

return self.execute(newRequest).futureResult.cascade(to: promise)

could look event simpler, wdyt?